-
Notifications
You must be signed in to change notification settings - Fork 513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RAC-225: DSM checkbox #12686
RAC-225: DSM checkbox #12686
Conversation
85c84f9
to
708cd2f
Compare
A new version of the staging env has been deployed 🎉 |
1 similar comment
A new version of the staging env has been deployed 🎉 |
Your visual regression tests are not passing. |
A new version of the staging env has been deployed 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GTM after fix
Also:
- We should add the animation present in the PIM https://github.com/akeneo/pim-community-dev/blob/master/src/Akeneo/Platform/Bundle/UIBundle/Resources/workspaces/shared/src/components/Checkbox.tsx#L5
- We could add a small margin between elements in the variation previews
A new version of the staging env has been deployed 🎉 |
* @TODO use grey140 instead of #11324d | ||
*/ | ||
|
||
const CheckboxContainer = styled.div < {checked: boolean, readOnly: boolean } > ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use the HTML <input type="checkbox">
element for semantical and accessibility purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I done this in a previous version this but with "undetermined" state i don't know how to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are to merge this PR, shouldn’t we need to seek approval from Stephan ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niiiice !
GTM after Steph’ has reviewed it!
akeneo-design-system/src/components/Checkbox/Checkbox.stories.mdx
Outdated
Show resolved
Hide resolved
Your visual regression tests are not passing. |
Your visual regression tests are not passing. |
Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
…nent and allow user to change theme
…e visual tests remove diffs when it fail
a6011e0
to
9f14582
Compare
Your visual regression tests are not passing. |
Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
A new version of the staging env has been deployed 🎉 |
a81a746
to
4d99cdb
Compare
Your visual regression tests are not passing. |
Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
A new version of the staging env has been deployed 🎉 |
1 similar comment
A new version of the staging env has been deployed 🎉 |
787f8f8
to
87e8548
Compare
A new version of the staging env has been deployed 🎉 |
const checkbox = getByText('Checkbox'); | ||
fireEvent.click(checkbox); | ||
|
||
expect(onChange).not.toBeCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check the checkbox is still checked ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, we didn't need to check if the checkbox is checked because it's a controlled component but I don't know @ValentinMumble what is your opinion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as it is a controlled component there shouldn't be any internal logic that could change the checked
prop so not really needed for me as well but as you wish
b2b2e98
to
728dcc5
Compare
Your visual regression tests are not passing. |
Co-authored-by: StevenVAIDIE <StevenVAIDIE@users.noreply.github.com>
A new version of the staging env has been deployed 🎉 |
Description (for Contributor and Core Developer)
This PR contain :
Definition Of Done (for Core Developer only)
Todo
: Pending / Work in progressOK
: Done / Validated-
: Not needed